Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide files excluded by .gitignore #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ti-mo
Copy link

@ti-mo ti-mo commented Jul 24, 2024

Fixes #14

This commit adds support for hiding files from output that have been excluded
by .gitignore and friends. I've implemented a few version of this and settled
on just `git ls-files` since it's likely going to be the most correct and
maintainable solution.

I've tried github.com/boyter/gocodewalker but it's a complex piece of machinery
and was much slower on a repo of 15k files (Cilium) than just git ls-files.
I also tried out github.com/ianlewis/go-gitignore directly, but it doesn't pick
up .gitignore files in subdirs automatically, nor the system-wide gitignore.

I figured since the overwhelming majority of users will be running this in CI
where Git will always be present, relying on the canonical .gitignore
implementation (git itself) is the safest option.

ti-mo added 2 commits July 24, 2024 09:26
This looks like a leftover from using another library for walking files.

Signed-off-by: Timo Beckers <[email protected]>
Fixes hmarr#14

This commit adds support for hiding files from output that have been excluded
by .gitignore and friends. I've implemented a few version of this and settled
on just `git ls-files` since it's likely going to be the most correct and
maintainable solution.

I've tried github.com/boyter/gocodewalker but it's a complex piece of machinery
and was much slower on a repo of 15k files (Cilium) than just git ls-files.
I also tried out github.com/ianlewis/go-gitignore directly, but it doesn't pick
up .gitignore files in subdirs automatically, nor the system-wide gitignore.

I figured since the overwhelming majority of users will be running this in CI
where Git will always be present, relying on the canonical .gitignore
implementation (git itself) is the safest option.

Signed-off-by: Timo Beckers <[email protected]>
@ti-mo ti-mo mentioned this pull request Jul 24, 2024
@rwese
Copy link

rwese commented Jul 24, 2024

Imho relying on the git cli command is not optimal.

Refactoring findRepositoryRoot would completely remove the need for exec breakout.

@ti-mo
Copy link
Author

ti-mo commented Jul 24, 2024

Honestly, I wouldn't reach for something much more complex. It's pretty involved to implement correctly, although the codeowners lib already has fnmatch -> regex compilation, which is a large chunk of what's needed. Hard to tell if it works exactly like git's, though.

If we use git as the source of truth for files to manage, there shouldn't be any surprises, it should just work. If someone wants to swap in a pure-Go implementation later, that's fine, but I personally don't think it's worth the engineering effort for a small tool like this.

@rwese
Copy link

rwese commented Oct 22, 2024

I am not the owner of this repo, but my 2 cents after reviewing the pr:

  1. test would be nice, could be accomplished by adding testdata/.gitignore and force add a testdata/ignored-file then have an example to check this.
  2. having a flag to enable it, to not break backward compatibility.

@icholy
Copy link

icholy commented Nov 19, 2024

having a flag to enable it, to not break backward compatibility.

@rwese Can you provide an example of something that this change would break?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect .gitignore
3 participants